Skip to content

Fix LogicalToBooleanRector: parenthesize assignments nested under unary/binary operators#8184

Open
TomasVotruba wants to merge 1 commit into
mainfrom
fix-logical-to-boolean-nested-assign-parens
Open

Fix LogicalToBooleanRector: parenthesize assignments nested under unary/binary operators#8184
TomasVotruba wants to merge 1 commit into
mainfrom
fix-logical-to-boolean-nested-assign-parens

Conversation

@TomasVotruba

Copy link
Copy Markdown
Member

Fixes #9807 and #9808.

LogicalToBooleanRector swaps and/or (low precedence) for &&/|| (high precedence). An assignment operand must then be wrapped in parentheses to keep the original semantics. This worked for a direct assignment operand, but not when the assignment was nested one level down — under a unary operator (!, ~, +, -, @, casts) or as an operand of a binary operator when it was a compound assignment (+=, -=, …). The result changed runtime behaviour, and ~$x = ... even produced a fatal TypeError.

Before / after

-!$x = 42 and true;        // !($x = 42)
+!$x = 42 && true;         // BUG: parses as !$x = (42 && true)
// input
$x = 0;
2 * $x += 42 and true;     // 2 * ($x += 42) → true, $x is 42

// before (broken)
2 * $x += 42 && true;      // 2 instead of true, $x is 1

// after (fixed)
2 * ($x += 42) && true;    // true, $x is 42

All reported forms now print correctly:

!($y = 42) && true;
~($w = 42) && true;
+($v = 42) && true;
-($z = 42) && true;
@($u = 42) && true;
(int) ($t = 42) && true;
2 * ($x += 42) && true;

Fix

In BetterStandardPrinter::wrapBinaryOpWithBrackets():

  • the binary-operand check now also covers AssignOp and AssignRef (was Assign only), fixing the compound-assignment case (#9808);
  • assignments nested directly under a unary operator are now forced to reprint, so the pretty-printer re-adds the parentheses by precedence (#9807).

Covered by 4 new fixtures. Existing printer + LogicalToBooleanRector tests stay green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant